fix(restore): do not clobber a backup slot the undo skipped - #13

Fixes #12

The bug

The redo direction writes the live file into the backup slot without checking whether that slot is already occupied:

// restore.go, OpUnlink redo
err = moveAny(field(0), field(1))

moveAny leads with os.Rename, which replaces the destination atomically, and its cross-device fallback opens the destination with O_TRUNC. Either way whatever was parked in that slot is gone.

The slot is occupied whenever the undo of that entry was skipped, which is an ordinary outcome rather than an error: the undo direction never overwrites, it skips and says why (path exists again, use --force to overwrite, original path occupied, use --force to overwrite, moved file is gone), and --only cherry-picks leave untouched entries in the same state. After such a partial undo the backup still holds the only copy of the deleted file. Redoing then renames the live file on top of it and counts the entry as restored.

Observed on main before this change, driving restore.Run directly:

after undo  : backup="the only copy of the deleted file" live="recreated since"
after redo  : live file is GONE
after redo  : backup="recreated since"  reported done=1 skipped=[]

Two files are unrecoverable — the deleted file whose only copy was in the slot, and the file that had been standing at the path — and nothing was reported as skipped.

That contradicts what the package promises in its own doc comment:

Both directions preserve data (files are swapped with or parked in the session store, never deleted), so undo and redo can toggle a session indefinitely.

OpRename's redo branch has the same shape: moveAny(new_, bak) overwrites a bak that a skipped undo left occupied.

The fix

A guard on each of the two redo paths that write into the session store, mirroring the checks the undo direction already performs on the working tree:

• OpUnlink redo, before moveAny(field(0), field(1))
• OpRename redo, before moveAny(new_, bak)

Both skip with an explanation instead of overwriting.

Neither guard is gated on --force, deliberately. On the undo side --force clobbers a file the user can see and has consciously decided to give up. Here the casualty is a backup inside the session store that the user has no way to inspect beforehand, and losing it makes that entry permanently unrecoverable. Skipping is also the semantically right answer and not merely the cautious one: an entry whose undo never happened is, as far as the working tree is concerned, already in its post-command state, so there is nothing for redo to re-apply.

Legitimate redo is unaffected. A successful undo moves the backup out of the slot, so the guard does not fire — e2e case 9 (rm → undo → undo redo → undo) covers exactly that path.

Tests

Two regression tests, placed beside the existing TestUndoUnlinkConflictSkipsWithoutForce whose shape they follow, one per guarded path. Each drives a genuinely skipped undo first, then redoes, then asserts the backup is still byte-for-byte what it was.

Before the fix:

$ go test ./internal/restore/ -run 'TestRedoUnlinkKeepsOccupiedBackup|TestRedoRenameKeepsOccupiedBackup' -v
=== RUN   TestRedoUnlinkKeepsOccupiedBackup
    restore_test.go:109: want 0 done / 1 skipped, got 1 done / 0 skipped
--- FAIL: TestRedoUnlinkKeepsOccupiedBackup (0.00s)
=== RUN   TestRedoRenameKeepsOccupiedBackup
    restore_test.go:203: want 0 done / 1 skipped, got 1 done / 0 skipped
--- FAIL: TestRedoRenameKeepsOccupiedBackup (0.00s)
FAIL
FAIL	github.com/edaywalid/undo/internal/restore	1.358s

After:

$ gofmt -l .
$ go vet ./...
$ go test ./...
ok  	github.com/edaywalid/undo/cmd/undo	1.118s
ok  	github.com/edaywalid/undo/internal/journal	(cached)
ok  	github.com/edaywalid/undo/internal/restore	0.525s
ok  	github.com/edaywalid/undo/internal/session	(cached)

gofmt -l . and go vet ./... printed nothing.

I also checked by hand that the ordinary round trips still complete after the guards — unlink undo → redo ends with the file deleted and the backup holding the original, rename undo → redo ends with the file back at its new path and bak holding b's clobbered original — using throwaway tests that are not part of this diff.

Not run locally: make test, i.e. test/e2e.sh and test/hook.sh. I am on macOS, where shim/undo_shim.c does not compile (O_LARGEFILE and other Linux/glibc-isms), so make cannot produce libundo.so and the shim-driven suites cannot start. This change is confined to the Go replay side and touches no shim code or shell hook; CI covers those suites on Linux.

Found and fixed with AI assistance. I reproduced the data loss locally, verified every cited line, and reviewed all conclusions before opening this.